Ensure connected SSHD session inherits PATH from parent process.#673
Ensure connected SSHD session inherits PATH from parent process.#673rgrunber wants to merge 1 commit intoche-incubator:mainfrom
Conversation
Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
rgrunber
requested review from
RomanNikitenko,
azatsarynnyy and
vitaliy-guliy
as code owners
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
📝 WalkthroughWalkthroughThe sshd startup script's environment variable injection logic was modified to export all environment variables from PID 1 and then explicitly append a PATH augmentation line, rather than filtering out PATH variables during the initial export phase. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
build/scripts/sshd.start (1)
70-72: Make shell init file generation idempotent.Using
>>on Line 70 and Line 72 appends duplicate exports on each restart when$HOMEis persistent. Prefer rewriting the generated block each run (or guarding with markers) to avoid unbounded growth and repeatedPATHentries.♻️ Example refactor
-env | sed 's|^|export |' >> $HOME/.profile -echo 'export PATH=$PATH:$HOME/bin' >> $HOME/.profile -cat $HOME/.profile >> $HOME/.bashrc +{ + env | sed 's|^|export |' + echo 'export PATH=$PATH:$HOME/bin' +} > "$HOME/.profile" +cat "$HOME/.profile" > "$HOME/.bashrc"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/scripts/sshd.start` around lines 70 - 72, The script currently appends environment exports and PATH lines unconditionally using the lines containing "env | sed 's|^|export |' >> $HOME/.profile" and "echo 'export PATH=$PATH:$HOME/bin' >> $HOME/.profile" which causes duplicate entries on each restart; change this to an idempotent update by writing a marked block to $HOME/.profile (or removing any existing marker-delimited block first) and then appending the single block (including the exported env lines and PATH update) once, and ensure you update .bashrc only once by replacing the "cat $HOME/.profile >> $HOME/.bashrc" behavior with either sourcing the marker block into .bashrc or adding a guard (grep for the marker or PATH entry) before modifying .bashrc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@build/scripts/sshd.start`:
- Around line 70-72: The script currently appends environment exports and PATH
lines unconditionally using the lines containing "env | sed 's|^|export |' >>
$HOME/.profile" and "echo 'export PATH=$PATH:$HOME/bin' >> $HOME/.profile" which
causes duplicate entries on each restart; change this to an idempotent update by
writing a marked block to $HOME/.profile (or removing any existing
marker-delimited block first) and then appending the single block (including the
exported env lines and PATH update) once, and ensure you update .bashrc only
once by replacing the "cat $HOME/.profile >> $HOME/.bashrc" behavior with either
sourcing the marker block into .bashrc or adding a guard (grep for the marker or
PATH entry) before modifying .bashrc.
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-673-amd64 |
1 participant
What issues does this PR fix?
echo $PATH | tr ':' '\n'and you'll see/home/tooling/go/binpresent./home/tooling/go/binon the path.The problem is the first line at
che-code/build/scripts/sshd.start
Line 68 in cf1d448
The single quotes mean the PATH references on the right-hand-side is never expanded and at runtime it's too late. We even avoid injecting it at
che-code/build/scripts/sshd.start
Lines 69 to 71 in cf1d448
This PR should fix this so that it inherits the PATH from the parent, but also includes the
$HOME/binlocation where the additional scripts needed for the SSHD setup are present.Summary by CodeRabbit